Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update .gitignore #176

Merged

Conversation

stankudrow
Copy link
Contributor

@stankudrow stankudrow commented Jul 20, 2024

Updating the project metadata files

For not having some features from the PR #171 completely gone, the desirable ones are moved here.

Changes in short:

  • the .gitignore file is updated according to the Python template from this repo
  • in the changed .md files the structure is changed to make the complaints of a markdown linter gone away + some links got fixed
  • minor changes in the makefile to ensure the chain of quality checks

@amyreese , are these changes welcome and good enough to be approved and merged?

makefile Outdated
@@ -19,12 +19,12 @@ format:

lint:
python -m flake8 $(PKG)
python -m mypy -p $(PKG)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that mypy checks are suitable for this section since type checks are part of linting stage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this, because mypy is testing the type annotations and finding errors in code, rather than just linting for best practices.

makefile Outdated
python -m ufmt check $(PKG)

test:
test: lint
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyreese , the rationale for this is not running tests on the code that foes not pass quality gates. The checks from mypy is a sort of linting stage. If the idea is grasped right, the test rule becomes depending on the lint rule that should be run before.

Let's discuss this addendum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much dislike this, because during development of new features, it's more important to know if tests are passing than whether or not all of the lints have been taken care of. Not being able to validate functionality of code because there's extra import or unused value is a really bad UX.

@stankudrow
Copy link
Contributor Author

stankudrow commented Jul 21, 2024

@amyreese , could the .pylint file be safely removed? I don't see the pylint package be used along the project. Are there any other files that are considered obsolete?

@stankudrow
Copy link
Contributor Author

stankudrow commented Aug 4, 2024

@amyreese , could this PR be merged first and then this one (#177)?

@stankudrow
Copy link
Contributor Author

@amyreese

@amyreese amyreese force-pushed the update-gitignore-and-other-md-files branch from 351dd73 to 7026ff2 Compare September 1, 2024 06:23
@amyreese amyreese changed the title Update .gitignore and other .md project files Update .gitignore Sep 1, 2024
@amyreese
Copy link
Member

amyreese commented Sep 1, 2024

I went ahead and reverted the makefile changes, as well as the markdown changes for now. Considering #183 as that is focused on sphinx compat rather than opinionated formatting.

@amyreese
Copy link
Member

amyreese commented Sep 1, 2024

@amyreese , could the .pylint file be safely removed? I don't see the pylint package be used along the project. Are there any other files that are considered obsolete?

I also removed that pylint config. Thank you for catching that.

@amyreese amyreese merged commit 538f169 into omnilib:main Sep 1, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants